Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a filter for memo and receiver field size #3765

Merged
merged 27 commits into from
Jan 17, 2024

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented Jan 10, 2024

Closes: #3766

Description

This PR adds two packet configurations:

  • ics20_max_memo_size which filters packets having a memo field bigger than the configured value
  • ics20_max_receiver_size which filters packets having a receiver field bigger than the configured value

These values default to the respective ones in ibc-go v8: https://github.com/cosmos/ibc-go/blob/04e14a7079e18323c64d4e00e12cf77708d24701/modules/apps/transfer/types/msgs.go#L15


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@ljoss17 ljoss17 changed the title Luca joss/memo and receiver filtering Add a filter for memo and receiver field size Jan 10, 2024
@ljoss17 ljoss17 requested review from romac and ancazamfir January 10, 2024 09:52
@ljoss17 ljoss17 marked this pull request as ready for review January 10, 2024 09:53
@ljoss17
Copy link
Contributor Author

ljoss17 commented Jan 10, 2024

Regarding the comments on the method validating the fields I ended up changing the logic in b535b44.

The method, now are_fields_valid will do the following:

  • If decoding succeeds and the fields are valid, continue with the relaying
  • If the decoding succeeds but the fields are not valid, output a debug log informing which packet failed the verification and skip the relaying for that packet
  • If the decoding fails, it considers the packet to be non-ICS20 so no memo and receiver field to validate, thus will return that the fields are valid and continue with the relaying

Let me know what you think about this new solution

romac
romac previously requested changes Jan 10, 2024
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
Ok((self.build_recv_packet(&event.packet, height)?, None))
match event
.packet
.are_fields_valid(self.max_memo_size, self.max_receiver_size)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not checking this earlier, i.e. for timeouts also? Same question for build_ack_from_recv_event()? Maybe even in the supervisor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could put it here https://github.com/informalsystems/hermes/blob/master/crates/relayer/src/link/relay_path.rs#L552 in the match cases:

  • IbcEvent::TimeoutPacket(_)
  • IbcEvent::SendPacket(ref event)
  • IbcEvent::WriteAcknowledgement(ref event)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it more, while it would be nice to have this in the supervisor it would not cover the startup and periodic clearing. So I guess we can check in relay_path but let's decide if we want to also apply for the other (than recv) message types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is now performed directly in generate_operational_data for all events which pertain to an ICS-04 packet: 3d5687e

@ancazamfir
Copy link
Collaborator

Left a few comments. In general I am wondering why we don't apply the checks for all msgs that contain a packet, i.e. recv, timeout and ack.

@romac
Copy link
Member

romac commented Jan 11, 2024

Good catch, we should indeed do that! Completely slipped my mind…

@romac romac mentioned this pull request Jan 15, 2024
11 tasks
ljoss17 and others added 2 commits January 15, 2024 14:18
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
@ljoss17
Copy link
Contributor Author

ljoss17 commented Jan 15, 2024

I refactored the memo and receiver field configuration and validation in this commit 451b578.

I added the option to disable the filter by having the configuration format as following: { enabled = true, size = "32KiB" }

The reason why I put the Ics20FieldSizeLimit struct in the relayer crate is because I feel like validation linked to the configuration shouldn't be in the relayer-types crate.

I only applied the verification 451b578#diff-072b872f1fbf4ea17384cecde28b35ee339faa9ce969ff9f767647a071da1e36R1405 as I wanted to have your opinion on this solution @ancazamfir @romac. If this seems like a good solution we can think more in detail to where we want to validate these fields

@ljoss17 ljoss17 marked this pull request as draft January 15, 2024 16:19
@romac romac dismissed their stale review January 16, 2024 15:10

Outdated

@romac
Copy link
Member

romac commented Jan 16, 2024

The check is now performed directly in generate_operational_data for all events which pertain to an ICS-04 packet: 3d5687e

@romac romac marked this pull request as ready for review January 16, 2024 16:05
@romac
Copy link
Member

romac commented Jan 16, 2024

The guide template checker is failing but that's unrelated to this PR. Will fix out of band.

Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks @ljoss17!

@romac romac merged commit 1bf1319 into master Jan 17, 2024
33 of 34 checks passed
@romac romac deleted the luca_joss/memo-and-receiver-filtering branch January 17, 2024 13:11
mergify bot added a commit to Agoric/agoric-sdk that referenced this pull request Nov 7, 2024
)

closes: #10249

## Description

This PR allows ICS-20 fungible token transfer packets to contain inbound receiver and outbound sender addresses formatted as either:
- a standard bech32 address (`"agoric1bech32addr"`), or
- a base account with optional subpaths and query parameters (`"agoric1bech32addr/opt/account?k=v&k2=v2"`)

In these cases, the "base address" is `"agoric1bech32addr"`, and tokens are transferred to that base address.  The original address can be parsed by a listening contract into "address parameters" to use at its discretion.

### Security Considerations

Should be no impact.  If a transfer listener does not explicitly look for the data in an inbound packet, it will behave as if there are no address parameters.

Careful review of the code is critical, though, since it touches the powerful `x/vtransfer` module.

### Scaling Considerations

Increases the scalability of contracts that would ordinarily have to maintain a pool of multiple Local Chain Account (LCA) addresses to have a unique ID to map to some parameters.  Now, contracts can create just one LCA and have the sender supply the data as part of a parameterized address.

The ICS-20 transfer addresses are strings limited to at most [2048 bytes, and potentially even smaller if explicitly configured](informalsystems/hermes#3765 (comment)) by a relayer operator.  Thus, the total length of the encoded base address and all the address parameters cannot exceed that address string limit, or the packet will not be forwarded.

### Documentation Considerations

Should be documented as a feature of `vtransfer`.

### Testing Considerations

Unit tests with address parameter data tests have been implemented, testing both the address parser code and the IBC end-to-end packet relay process through the `x/vtransfer` module.

### Upgrade Considerations

This is a chain-level, backwards-compatible change, so no upgrade aspects must be addressed.  However, Agoric contracts will probably need to import an URL-parsing module to parse URL escaping and query parameters syntax (since the `globalThis.URL` constructor is not currently available in the SwingSet vat environment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a configuration to ignore packets with memo or receiver fields too big
3 participants